Fix path traversal in profile operations#55
Conversation
Reject profile names containing path separators or traversal sequences to prevent arbitrary filesystem delete/create/symlink. Fixes Microck#54
|
@AAtomical is attempting to deploy a commit to the Projects Team on Vercel. A member of the Team first needs to authorize it. |
|
This PR prevents profile operations from acting on path-traversal names by rejecting separators and
WalkthroughAdded Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/profile-manager.js`:
- Around line 9-14: Update safeName to reject "." and enforce that the resolved
profile path is a direct child of PROFILES_DIR before deleteProfile or
activateProfile performs filesystem operations; add a regression test covering
"." and path containment.
- Around line 68-69: Require authentication and authorization for profile
mutations before invoking deleteProfile or the activation operation. Update the
DELETE /api/profiles/:name and POST /api/profiles/:name/activate handlers to
validate the requester’s permission to manage the supplied profile, while
preserving safeName validation and rejecting unauthorized requests before any
deletion or OPENCODE_DIR change occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: cfeb784a-0b64-41d0-b275-fa194984f601
📒 Files selected for processing (1)
server/profile-manager.js
| function safeName(name) { | ||
| if (!name || name.includes('/') || name.includes('\\') || name.includes('..')) { | ||
| throw new Error('Invalid profile name'); | ||
| } | ||
| return name; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: External
Reject . and enforce path containment.
safeName('.') passes validation, but path.join(PROFILES_DIR, '.') resolves to PROFILES_DIR. Consequently, deleteProfile('.') recursively deletes the entire profiles directory, and activateProfile('.') can point OPENCODE_DIR at the profiles root. Reject . and verify the resolved target is a direct child before filesystem operations; add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/profile-manager.js` around lines 9 - 14, Update safeName to reject "."
and enforce that the resolved profile path is a direct child of PROFILES_DIR
before deleteProfile or activateProfile performs filesystem operations; add a
regression test covering "." and path containment.
| function deleteProfile(name) { | ||
| safeName(name); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authorization Bypass (CWE-862): Missing Authorization
Reachability: External
Require authorization before profile deletion and activation.
The supplied DELETE /api/profiles/:name and POST /api/profiles/:name/activate handlers pass request-controlled names directly to these methods without an ownership or permission check. safeName prevents traversal but still lets any unauthenticated caller delete another non-active profile or switch the global OPENCODE_DIR. Enforce authentication and authorization before invoking these operations.
Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/profile-manager.js` around lines 68 - 69, Require authentication and
authorization for profile mutations before invoking deleteProfile or the
activation operation. Update the DELETE /api/profiles/:name and POST
/api/profiles/:name/activate handlers to validate the requester’s permission to
manage the supplied profile, while preserving safeName validation and rejecting
unauthorized requests before any deletion or OPENCODE_DIR change occurs.
|
@AAtomical Thanks for the PR! Feel free to fix the issues notified by Greptile and CodeRabbit, afterwards I will merge to main. |
…to localhost Addresses CodeRabbit/Greptile review on Microck#55: - safeName now requires the name to resolve to a DIRECT child of PROFILES_DIR, rejecting '.', '..', path separators, absolute and empty names. Previously safeName('.') passed and deleteProfile('.') wiped the entire profiles dir. - Bind the server to 127.0.0.1 so the unauthenticated profile/auth endpoints are not reachable from the network (CWE-862 external reachability).
…to localhost Addresses CodeRabbit/Greptile review on Microck#55: - safeName now requires the name to resolve to a DIRECT child of PROFILES_DIR, rejecting '.', '..', path separators, absolute and empty names. Previously safeName('.') passed and deleteProfile('.') wiped the entire profiles dir. - Bind the server to 127.0.0.1 so the unauthenticated profile/auth endpoints are not reachable from the network (CWE-862 external reachability).
e1f7c6f to
c3e17fe
Compare
Covers '.' and traversal names rejected, direct-child names accepted (CodeRabbit/Greptile follow-up on Microck#55). Exports safeName for the test.
Covers '.' and traversal names rejected, direct-child names accepted (CodeRabbit/Greptile follow-up on Microck#55). Exports safeName for the test.
d63ca39 to
3212daf
Compare
Covers '.' and traversal names rejected, direct-child names accepted (CodeRabbit/Greptile follow-up on Microck#55). Exports safeName for the test.
3212daf to
9ae0139
Compare
Reject profile names containing path separators or traversal sequences to prevent arbitrary filesystem delete/create/symlink.
Fixes #54
Greptile Summary
This PR hardens profile filesystem operations against unsafe profile names. The main changes are:
safeNamevalidation before profile create, delete, and activate operations.127.0.0.1.Confidence Score: 5/5
Safe to merge with low risk.
Profile operations now validate names before path construction, and the previously reported dot-name case is covered by validation and tests.
No files require special attention.
What T-Rex did
Important Files Changed
safeNamevalidation before profile create/delete/activate operations to reject traversal, separators, empty values, and non-direct child paths.127.0.0.1and updates the startup URL accordingly.Sequence Diagram
sequenceDiagram participant Client participant API as Profile API participant PM as profile-manager participant FS as Filesystem Client->>API: create/delete/activate profile(name) API->>PM: profile operation(name) PM->>PM: safeName(name) alt invalid name PM-->>API: throw Invalid profile name API-->>Client: 400 error else valid direct-child name PM->>FS: create/remove/symlink under PROFILES_DIR FS-->>PM: success PM-->>API: "{ success: true }" API-->>Client: success response endReviews (2): Last reviewed commit: "Add regression test for safeName path co..." | Re-trigger Greptile